Skip to content
New issue

Have a question about this project? Sign up for a free GitHub account to open an issue and contact its maintainers and the community.

By clicking “Sign up for GitHub”, you agree to our terms of service and privacy statement. We’ll occasionally send you account related emails.

Already on GitHub? Sign in to your account

[10.x] Anonymous component bound attribute values are evaluated twice #50403

Merged
merged 8 commits into from
Mar 8, 2024
Merged

[10.x] Anonymous component bound attribute values are evaluated twice #50403

merged 8 commits into from
Mar 8, 2024

Conversation

danharrin
Copy link
Contributor

@danharrin danharrin commented Mar 7, 2024

When passing bound attributes to an anonymous component, the values of those attributes get evaluated twice. This can result in a performance issue if the evaluation contains a heavy operation.

For example:

<x-input :label="$getLabel()" />

Before this PR, $getLabel() was called twice. Now, the second time we require the value, we reuse it value from the first time it was evaluated.

In this PR, a new $componentData variable is introduced to store the data when it is first evaluated. I have modified the tag compiler to ensure that existing $componentData variables in userland code get reassigned to a new variable, similarly to what happens with the $component and $attributes variables already.

Currently, I can only find compilation tests for anonymous Blade components. If you can point me to an area where the end developer experience for anonymous Blade components is tested, I can introduce a new test to ensure the function passed to the bound attribute does not get called more than once.

@danharrin danharrin changed the title [10.x]: Anonymous component bound attribute values are evaluated twice [10.x] Anonymous component bound attribute values are evaluated twice Mar 7, 2024
@taylorotwell
Copy link
Member

Sheesh, this is a scary PR. 😅 Can't the heavy operation just be done in a controller or at the top of the script and then passed to the component as a value?

@danharrin
Copy link
Contributor Author

danharrin commented Mar 7, 2024

It can be worked around, but in my opinion that isn't an ideal DX, as it's just a one-time use variable at that point, and an extra line you need to include for each attribute.

Imagine you have a class-based component with a nested anonymous component, you can call methods on the class based component using $method() closure syntax and pass them directly in to the child: sometimes there isn't a controller. Also with Livewire if you are calling methods on the component with $this->method(), it's not always ideal to pass these in using render() if the data is only required conditionally.

It is a bit scary, but I'm pretty sure it's ok and the changes are actually pretty simple (there are just a lot of compiled snapshot tests that touch this code).

@danharrin
Copy link
Contributor Author

Maybe safer to introduce in 11 if you're hesitant?

Comment on lines 797 to 804
if (
(! $escapeBound) ||
(! isset($this->boundAttributes[$attribute])) ||
($value === 'true') ||
is_numeric($value)
) {
return "'{$attribute}' => {$value}";
}
Copy link
Contributor Author

Choose a reason for hiding this comment

The reason will be displayed to describe this comment to others. Learn more.

This is the same as the existing check on line 779

@taylorotwell taylorotwell merged commit f2bff1c into laravel:10.x Mar 8, 2024
24 checks passed
@taylorotwell
Copy link
Member

taylorotwell commented Mar 8, 2024

Will give it a shot. Thanks.

@danharrin danharrin deleted the fix/anonymous-component-attribute-multiple-evaluation branch March 8, 2024 23:47
@dash8x
Copy link

dash8x commented Mar 12, 2024

This pull request breaks passing bound attributes to components if the attribute is not defined as a data attribute of the component.

For example if the component class was:

class Input extends Component
{
public function __construct(
        public string $type = 'text'
    )
}

and the blade view was:

<x-input :name="$getName()" />

Then the name attribute is null when rendering the component attributes.

@danharrin
Copy link
Contributor Author

danharrin commented Mar 12, 2024

Hi, what do you mean by "defined as a data attribute"? In the attribute bag?

@dash8x
Copy link

dash8x commented Mar 12, 2024

@danharrin I mean now bound attributes only work if you have defined the attribute in the constructor of the Component class. Otherwise the attribute value is always null. Since the $componentData array does not include those attributes.

@danharrin
Copy link
Contributor Author

How are you accessing the name inside the input? Through the bag or through a variable?

@dash8x
Copy link

dash8x commented Mar 12, 2024

Through the bag.

<input type="{{ $type }}" {{ $attributes }} />

@danharrin
Copy link
Contributor Author

Gotcha, I am going to look at this and submit a PR. I'm not a user of class based components and the test suite doesn't contain any end user component tests

dash8x added a commit to dash8x/framework that referenced this pull request Mar 12, 2024
- Fixed bound attributes not rendering for non-data attributes, see pull request laravel#50403
@danharrin
Copy link
Contributor Author

Fix reference: #50479

@devajmeireles
Copy link
Contributor

LOL @danharrin , I was writting an issue about the same report of @dash8x.

@driesvints
Copy link
Member

Thanks. Unfortunately we'll have to revert this one as it's breaking for too many people: #50482

Sign up for free to join this conversation on GitHub. Already have an account? Sign in to comment
Labels
None yet
Projects
None yet
Development

Successfully merging this pull request may close these issues.

5 participants